Migrated ESASky module to use EsaTap/PyVO instead of TAPPlus#3572
Migrated ESASky module to use EsaTap/PyVO instead of TAPPlus#3572imbasimba wants to merge 5 commits intoastropy:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3572 +/- ##
==========================================
+ Coverage 73.10% 73.17% +0.06%
==========================================
Files 224 224
Lines 20839 20836 -3
==========================================
+ Hits 15234 15246 +12
+ Misses 5605 5590 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
Big picture looks OK, but we will need to think a bit more about what deprecations will need to be done; or if we want to do a clear break and call this a major enough refactor where we don't provide non-breaking API changes.
| urlBase = _config.ConfigItem( | ||
| 'https://sky.esa.int/esasky-tap', | ||
| 'ESASky base URL') | ||
|
|
||
| timeout = _config.ConfigItem( | ||
| 1000, | ||
| 'Time limit for connecting to template_module server.') | ||
|
|
||
| row_limit = _config.ConfigItem( | ||
| 10000, | ||
| 'Maximum number of rows returned (set to -1 for unlimited).') |
There was a problem hiding this comment.
These are public attributes, so I don't think we can remove them without doing a deprecation first.
There was a problem hiding this comment.
(you can make them an alias to the new variables along with a warning)
| def __init__(self, tap_handler=None): | ||
| super().__init__() | ||
| def __init__(self, *, show_messages=False, auth_session=None, tap_url=None): | ||
| super().__init__(auth_session=auth_session, tap_url=tap_url) |
There was a problem hiding this comment.
OK, so this PR will add some backwards incompatible api changes. If we clearly communicate that in the changelog, I think we can be OK with it even if we decide of not doing a deprecation, but we definitely need to communicate that.
cc @keflavich
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
I see a lot of remote test failures, changing the review status to make sure those will get fixed up before we merge this.
2 participants
Following the merge of #3511, here is the migration for the ESASky module